Control Sandbox SMTP server based on DynamoDB config, save config on app register from Saleor data#2292
Conversation
🦋 Changeset detectedLatest commit: 77848c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
8 Skipped Deployments
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2292 +/- ##
==========================================
+ Coverage 37.21% 37.40% +0.19%
==========================================
Files 1015 1019 +4
Lines 65896 66131 +235
Branches 3384 3429 +45
==========================================
+ Hits 24522 24738 +216
- Misses 40999 41018 +19
Partials 375 375
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Differences Found✅ No packages or licenses were added. SummaryExpand
|
There was a problem hiding this comment.
Pull request overview
This PR introduces DynamoDB-backed, per-tenant control over the SMTP fallback (“Sandbox SMTP”) behavior, including persisting fallback settings from Saleor /register additional_data, and using that config during email sending.
Changes:
- Bump
@saleor/app-sdkto1.7.2-dev.1and adjust pnpm trust policy exclusions. - Add DynamoDB-backed fallback SMTP config repository/service, persist config on app registration, and expose/manage it via tRPC + UI.
- Update email sending use case to optionally redirect fallback emails to a configured address.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Updates app-sdk version and pnpm trust policy exclusions for the dev build. |
| pnpm-lock.yaml | Lockfile update to reflect the app-sdk version bump and dependency graph changes. |
| apps/smtp/src/pages/configuration/index.tsx | Surfaces fallbackRedirectEmail in the fallback section UI and triggers fallback settings mutation. |
| apps/smtp/src/pages/api/register.ts | Saves fallback config on register (from additional_data) before creating webhooks. |
| apps/smtp/src/modules/trpc/protected-client-procedure-with-services.ts | Injects fallback config service into tRPC context. |
| apps/smtp/src/modules/smtp/configuration/smtp-configuration.service.ts | Removes fallback settings methods from metadata-based SMTP configuration service. |
| apps/smtp/src/modules/smtp/configuration/smtp-configuration.router.ts | Switches fallback settings endpoints to DynamoDB-backed service and extends API with redirect email. |
| apps/smtp/src/modules/fallback-smtp/on-auth-apl-saved.ts | New helper to parse and persist fallback config from register payload. |
| apps/smtp/src/modules/fallback-smtp/on-auth-apl-saved.test.ts | Unit tests for saving fallback config on register. |
| apps/smtp/src/modules/fallback-smtp/fallback-smtp-service.ts | New service that wires env gating + DynamoDB repository for fallback config. |
| apps/smtp/src/modules/fallback-smtp/fallback-smtp-service.test.ts | Unit tests for fallback service behavior and caching. |
| apps/smtp/src/modules/fallback-smtp/fallback-smtp-config-repository.ts | New DynamoDB repository for fallback config storage. |
| apps/smtp/src/modules/fallback-smtp/fallback-smtp-config-repository.test.ts | Unit tests for DynamoDB repository behaviors and defaults. |
| apps/smtp/src/modules/fallback-smtp/fallback-register-data.ts | New parser/validator for additional_data fallback config. |
| apps/smtp/src/modules/fallback-smtp/fallback-register-data.test.ts | Unit tests for register additional_data parsing behavior. |
| apps/smtp/src/modules/event-handlers/use-case/send-event-messages.use-case.ts | Uses DynamoDB-backed fallback config; supports redirecting recipient when configured. |
| apps/smtp/src/modules/event-handlers/use-case/send-event-messages.use-case.test.ts | Updates tests to cover new fallback config source and redirect behavior. |
| apps/smtp/src/modules/event-handlers/use-case/send-event-messages.use-case.factory.ts | Injects fallback config service into the use case. |
| apps/smtp/src/modules/app-configuration/ui/configuration-fallback.tsx | Displays fallback redirect info when configured. |
| apps/smtp/scripts/setup-dynamodb.ts | Adds local DynamoDB table setup script. |
| apps/smtp/package.json | Adds setup-dynamodb script. |
| apps/smtp/README.md | Documents fallback SMTP + DynamoDB usage and local setup steps. |
| apps/smtp/.env.example | Adds DynamoDB-related env vars for fallback SMTP mode. |
| .devcontainer/smtp/docker-compose.yml | Adds a local dynamodb-local service for development. |
| .changeset/metal-crews-chew.md | Changeset entry describing the new fallback config behavior. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| "saleor-app-smtp": minor | ||
| --- | ||
|
|
||
| App will now parse `additional_data` received from Saleor in /register endpoint and use it to save Sandbox SMTP configuration in DynamoDB. This configuration toggles if SMTP Sandbox server should be used and if all sent email receipient should be overwritten to a single email adderss. |
There was a problem hiding this comment.
Spelling fixes needed in the changeset text (e.g. "receipient" → "recipient", "adderss" → "address"). These typos will end up in the published changelog.
| App will now parse `additional_data` received from Saleor in /register endpoint and use it to save Sandbox SMTP configuration in DynamoDB. This configuration toggles if SMTP Sandbox server should be used and if all sent email receipient should be overwritten to a single email adderss. | |
| App will now parse `additional_data` received from Saleor in /register endpoint and use it to save Sandbox SMTP configuration in DynamoDB. This configuration toggles if SMTP Sandbox server should be used and if all sent email recipient should be overwritten to a single email address. |
| const fallbackConfigRepo = new FallbackSmtpService({ | ||
| saleorApiUrl: ctx.saleorApiUrl, | ||
| }); | ||
|
|
||
| const result = await next({ | ||
| ctx: { | ||
| smtpConfigurationService, | ||
| featureFlagService, | ||
| fallbackConfigRepo, | ||
| }, |
There was a problem hiding this comment.
fallbackConfigRepo is an instance of FallbackSmtpService (a service) rather than a repository. Renaming this (and the corresponding ctx field) to fallbackConfigService (or similar) would make the dependency clearer and avoid confusion with the underlying FallbackSmtpConfigRepository.
| * For existing installations before this check was introduced, | ||
| * we allow using SMTP fallback without redirect email restrictions | ||
| */ | ||
| if (!result.Item) { | ||
| return { fallbackEnabled: true, fallbackRedirectEmail: null }; |
There was a problem hiding this comment.
fetchConfig() returns { fallbackEnabled: true, fallbackRedirectEmail: null } when the DynamoDB item is missing. This makes fallback SMTP enabled by default for any tenant without an explicit config record, which contradicts the goal of controlling fallback via DynamoDB and also differs from the previous default (useSaleorSmtpFallback: false). Consider returning an error / “not configured” state when Item is missing, or defaulting fallbackEnabled to false and handling legacy installs via an explicit migration path.
| * For existing installations before this check was introduced, | |
| * we allow using SMTP fallback without redirect email restrictions | |
| */ | |
| if (!result.Item) { | |
| return { fallbackEnabled: true, fallbackRedirectEmail: null }; | |
| * When no configuration is stored in DynamoDB, | |
| * fallback SMTP is disabled by default. | |
| */ | |
| if (!result.Item) { | |
| return { fallbackEnabled: false, fallbackRedirectEmail: null }; |
| if (!fallbackSmtpConfig) { | ||
| this.logger.info("Fallback SMTP env vars are not configured"); | ||
|
|
||
| return err([ | ||
| new SendEventMessagesUseCase.InvalidEmailAddressError( | ||
| "Received an invalid email address: couldn't determine the domain", | ||
| new SendEventMessagesUseCase.FallbackNotConfiguredError( | ||
| "Fallback enabled but env vars are not configured", | ||
| { | ||
| props: { channelSlug, event, recipientEmail }, | ||
| props: { channelSlug, event }, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
When fallback SMTP env vars are missing, the returned error message says "Fallback enabled but env vars are not configured", but at this point the code hasn’t checked whether fallback is enabled in DynamoDB yet. This can be misleading in logs/UI. Consider changing the message to reflect the actual condition (e.g. "Fallback SMTP env vars are not configured").
| This feature is controlled by: | ||
|
|
||
| 1. **Fallback SMTP env vars** (`FALLBACK_SMTP_HOST`, etc.) - set by Saleor Cloud on the deployment, defining the actual SMTP server credentials | ||
| 2. **Per-tenant config in DynamoDB** - stores `fallbackEnabled` (boolean) and optional `fallbackRedirectEmail` (string) per installation |
There was a problem hiding this comment.
The README says the DynamoDB config is stored “per installation”, but the implementation uses DynamoMainTable.getPrimaryKeyScopedToSaleorApiUrl({ saleorApiUrl }), which is tenant-scoped (persists across reinstalls). Either adjust the README wording or change the PK strategy to match the intended scoping.
| 2. **Per-tenant config in DynamoDB** - stores `fallbackEnabled` (boolean) and optional `fallbackRedirectEmail` (string) per installation | |
| 2. **Per-tenant config in DynamoDB** - stores `fallbackEnabled` (boolean) and optional `fallbackRedirectEmail` (string) per Saleor API URL (tenant), shared across app reinstalls |
| 1. **Fallback SMTP env vars** (`FALLBACK_SMTP_HOST`, etc.) - set by Saleor Cloud on the deployment, defining the actual SMTP server credentials | ||
| 2. **Per-tenant config in DynamoDB** - stores `fallbackEnabled` (boolean) and optional `fallbackRedirectEmail` (string) per installation | ||
|
|
||
| When a store installs the app, Saleor can pass `additional_data` with `fallbackEnabled` and `fallbackRedirectEmail`. The app validates and stores this in DynamoDB. If no `additional_data` is provided or `fallbackEnabled` is missing, fallback is not configured. |
There was a problem hiding this comment.
README states: "If no additional_data is provided or fallbackEnabled is missing, fallback is not configured." Current implementation defaults to fallbackEnabled: true when the DynamoDB item is missing, and parseFallbackRegisterData returns a disabled config (and saves it) when fallbackEnabled is missing but additional_data is present. Consider updating the docs to match the actual behavior, or adjust the code to enforce the documented “not configured” state.
| When a store installs the app, Saleor can pass `additional_data` with `fallbackEnabled` and `fallbackRedirectEmail`. The app validates and stores this in DynamoDB. If no `additional_data` is provided or `fallbackEnabled` is missing, fallback is not configured. | |
| When a store installs the app, Saleor can pass `additional_data` with `fallbackEnabled` and `fallbackRedirectEmail`. The app validates and stores this in DynamoDB. If no `additional_data` is provided, the app assumes fallback is enabled (subject to the env vars being set). If `additional_data` is provided but `fallbackEnabled` is missing, the app stores a disabled fallback configuration by default. |
|
|
||
| trustPolicy: no-downgrade | ||
|
|
||
| # TODO: Remove one once released |
There was a problem hiding this comment.
Minor wording issue in the TODO: "Remove one once released" is unclear/grammatically incorrect. Consider rephrasing to something like "Remove once released" or specify what should be removed.
| # TODO: Remove one once released | |
| # TODO: Remove this exclusion once the stable version is released |
| @@ -98,6 +98,7 @@ const ConfigurationPage: NextPage = () => { | |||
| fallbackSettingsMutation.mutate({ useSaleorSmtpFallback: newValue }); | |||
There was a problem hiding this comment.
updateFallbackSmtpSettings mutation is called without fallbackRedirectEmail, but the router currently treats an omitted value as null (clears it). This means toggling the checkbox will unintentionally wipe an existing redirect email. Consider passing the currently loaded fallbackRedirectEmail along with useSaleorSmtpFallback, or changing the API to support partial updates without clearing fields when they are omitted.
| fallbackSettingsMutation.mutate({ useSaleorSmtpFallback: newValue }); | |
| if (!fallbackSettingsQuery.data) { | |
| return; | |
| } | |
| fallbackSettingsMutation.mutate({ | |
| useSaleorSmtpFallback: newValue, | |
| fallbackRedirectEmail: fallbackSettingsQuery.data.fallbackRedirectEmail, | |
| }); |
| return ctx.fallbackConfigRepo | ||
| .setFallbackConfig({ | ||
| fallbackEnabled: input.useSaleorSmtpFallback, | ||
| fallbackRedirectEmail: input.fallbackRedirectEmail ?? null, | ||
| }) | ||
| .match( | ||
| (v) => v, | ||
| () => ({ | ||
| useSaleorSmtpFallback: input.useSaleorSmtpFallback, | ||
| fallbackRedirectEmail: input.fallbackRedirectEmail ?? null, | ||
| }), |
There was a problem hiding this comment.
fallbackRedirectEmail is optional(), but the implementation writes fallbackRedirectEmail: input.fallbackRedirectEmail ?? null. When clients omit the field (e.g. current UI), this will clear any previously stored redirect email. If the intent is “leave unchanged when omitted”, handle undefined separately (read existing config first, or require the field in input).
|
|
||
| const configResult = await ctx.fallbackConfigRepo.getFallbackConfig(); | ||
|
|
||
| return { isConfigured: configResult.isOk() }; |
There was a problem hiding this comment.
isFallbackSmtpConfigured currently reports isConfigured: true whenever getFallbackConfig() returns Ok. Since the repository returns Ok even when the item doesn’t exist (default config), this endpoint will treat “no DynamoDB record” as “configured”. If “configured” is meant to indicate an explicit per-tenant decision from additional_data / DynamoDB, consider checking for item existence (or exposing a dedicated isConfigured method) rather than relying on isOk().
| return { isConfigured: configResult.isOk() }; | |
| if (configResult.isErr()) { | |
| // If the configuration cannot be loaded, treat it as not explicitly configured. | |
| return { isConfigured: false }; | |
| } | |
| const config = configResult.value; | |
| const isConfigured = | |
| config.fallbackEnabled === true || | |
| config.fallbackRedirectEmail !== null && config.fallbackRedirectEmail !== undefined; | |
| return { isConfigured }; |
|
@witoszekdev, in the context of the work we discussed these changes are no longer necessary. |
Adds configuration in DynamoDB for controlling the Sandbox SMTP server.